Changes to TransitDataProviderFilter #6850
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6850 +/- ##
==========================================
Coverage 72.10% 72.11%
- Complexity 19670 19675 +5
==========================================
Files 2125 2125
Lines 79531 79529 -2
Branches 8051 8051
==========================================
+ Hits 57348 57352 +4
+ Misses 19346 19341 -5
+ Partials 2837 2836 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ) | ||
| ); | ||
|
|
||
| assertTrue(filter.tripTimesPredicate(tripTimesMatchingSubModeMatchingAgency, true)); |
There was a problem hiding this comment.
This didn't work in practice in the previous code since the filter would have returned false at the filter.tripPatternPredicate(p) check.
habrahamsson-skanetrafiken
added
the
+Skip Changelog
habrahamsson-skanetrafiken
changed the title
habrahamsson-skanetrafiken
requested review from
leonardehrenfried and
t2gran
There was a problem hiding this comment.
I had a call with @habrahamsson-skanetrafiken where we went through these changes. It's difficult to reason about all of this but after looking at it together I'm satisfied that this improves the code and doesn't introduce new bugs.
| if (filter.matchTripPattern(tripPatternForDate.getTripPattern().getPattern())) { | ||
| return true; | ||
| if (filter.matchTripPattern(tripPattern)) { | ||
| var withFilters = hasSubModeFilters && tripPattern.getContainsMultipleModes(); |
There was a problem hiding this comment.
withFilters should probably have a better name. What about applyTripTimesFilters ? This apply to the tripTimesPredicate method argument as well.
| return true; | ||
| if (filter.matchTripPattern(tripPattern)) { | ||
| var withFilters = hasSubModeFilters && tripPattern.getContainsMultipleModes(); | ||
| return tripTimes -> this.tripTimesPredicate(tripTimes, withFilters); |
There was a problem hiding this comment.
| return tripTimes -> this.tripTimesPredicate(tripTimes, withFilters); | |
| return tripTimes ->tripTimesPredicate(tripTimes, withFilters); |
Avoid using this. unless it is needed.
|
|
||
| @Override | ||
| public boolean tripTimesPredicate(TripTimes tripTimes, boolean withFilters) { | ||
| private boolean tripTimesPredicate(TripTimes tripTimes, boolean withFilters) { |
| @Override | ||
| public boolean tripPatternPredicate(TripPatternForDate tripPatternForDate) { | ||
| @Nullable | ||
| public Predicate<TripTimes> getTripFilter(TripPattern tripPattern) { |
There was a problem hiding this comment.
Consider renaming this to createTripFilter
|
You have to remove unused imports. |
habrahamsson-skanetrafiken
merged commit c08d739
into
opentripplanner:dev-2.x
3 participants
Summary
This is mostly a refactor, but also fixes a bug in the filtering.
We've got an upcoming feature where we will want to use the TransitDataProviderFilter in a few new places. Currently the api of the filter interface is a bit convoluted since the caller has to decide whether to enable parts of the filtering depending on a complex set of circumstances, this causes tight coupling between the caller and TransitDataProviderFilter and makes it easy to make a mistake when using the TransitDataProviderFilter.
This PR changes the api of TransitDataProviderFilter so that the caller has to do the correct thing and first provide a TripPattern and then a TripTimes object.
By switching to this new api in the tests I also found a bug that is fixed.
I also found another bug that will be difficult to fix without some heavier rewriting of the filtering logic. I left it unfixed and made an
@Ignoredunit test to illustrate the problem.Unit tests
The tests for TransitDatProviderFilter are updated to use both a TripPattern and a TripTimes object instead of testing the pattern and tripTimes filtering in isolation. This reflects how the filter class is actually used.
Documentation
I added some javadoc to undocumented methods.
Bumping the serialization version id
Nope